Skip to content

Replace OpenSimAddTests macro with equivalent OpenSimAddTest for individual tests - #4466

Merged
nickbianco merged 2 commits into
mainfrom
add_test_macro
Oct 7, 2026
Merged

nickbianco merged 2 commits into
mainfrom
add_test_macro

Conversation

@nickbianco

@nickbianco nickbianco commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Fixes issue #4441

Brief summary of changes

The testing macro OpenSimAddTests has been changed to OpenSimAddTest, a mostly equivalent macro for adding a single test target at a time. This change fixes #4441, since now test resources are explicitly assigned to each test target. This also makes it easier to identify which resources are associated with each test just be looking at each test directory's CMakeLists file (rather than parsing through everything individual test).

OpenSimAddTest automatically links to osimTesting and Catch2::Catch2WithMain; other libraries can be linked to via the LINKLIBS argument, as before. Other arguments include RESOURCES, to specify test resources, EXTRA_SOURCES to specify any related .h or .cpp files, ENVIRONMENT to provide test-specific environment variables, and DISABLED to indicate to CTest to skip the test when running the test suite.

Other changes include:

  • Usage of MocoAddTest in OpenSim/Moco/tests/CMakeLists.txt has been replaced with OpenSimAddTest.
  • testContext.cpp has been converted to the Catch2 framework, which is necessary since the new macro automatically links to osimTesting and Catch2::Catch2WithMain.

Testing I've completed

Ran tests locally; CI.

Looking for feedback on...

CHANGELOG.md (choose one)

  • no need to update because...internal test updates.

This change is Reviewable

@nickbianco
nickbianco marked this pull request as ready for review October 1, 2026 19:44
@nickbianco
nickbianco requested a review from aymanhab October 5, 2026 18:56
@nickbianco

Copy link
Copy Markdown
Member Author

@aymanhab would you be able to review this one?

@aymanhab aymanhab left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Just had a few questions, clarifications but looks great. Nice work @nickbianco

@aymanhab reviewed 26 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on nickbianco).


Applications/CMC/tests/CMakeLists.txt line 8 at r1 (raw file):

                     resources/*.xml
                     resources/*.sto
                     resources/*.mot

Do we really need to keep around these vtp files?


cmake/OpenSimMacros.cmake line 466 at r1 (raw file):

        # Add the test.
        add_test(NAME ${OSIMTEST_NAME} COMMAND ${OSIMTEST_NAME} ${test_args})

At some point we used symlink to avoid copying resource files, is that still the behavior? If not, are there any noticeable effects on time or space to run the tests?


cmake/OpenSimMacros.cmake line 476 at r1 (raw file):

                ENVIRONMENT ${OSIMTEST_ENVIRONMENT})

        # Copy test resources.

Do we know if any of these commented out flags were actually used? Should we keep these options easy to toggle?


OpenSim/Common/tests/CMakeLists.txt line 22 at r1 (raw file):

if(WITH_EZC3D)
    OpenSimAddTest(NAME testC3DFileAdapter
            LINKLIBS ${OSIM_COMMON_TEST_LIBS}

Glad to see this reversed 👍


OpenSim/Simulation/SimbodyEngine/tests/CMakeLists.txt line 7 at r1 (raw file):

        LINKLIBS osimSimulation osimCommon osimAnalyses
        RESOURCES resources/testJointConstraints.osim
        )

May not be related but I'd think tests under Simulation library would not need to link osimAnalyses

@nickbianco nickbianco left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aymanhab! I've addressed all comments.

@nickbianco partially reviewed 26 files and made 6 comments.
Reviewable status: 25 of 26 files reviewed, 5 unresolved discussions (waiting on aymanhab).


Applications/CMC/tests/CMakeLists.txt line 8 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Do we really need to keep around these vtp files?

Probably not. I think it would make sense to defer picking out unnecessary files to later PRs. This change should make it easier to do that incrementally.


cmake/OpenSimMacros.cmake line 466 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

At some point we used symlink to avoid copying resource files, is that still the behavior? If not, are there any noticeable effects on time or space to run the tests?

Ah, looks like I lost that behavior in this edit. Adding it back.


cmake/OpenSimMacros.cmake line 476 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Do we know if any of these commented out flags were actually used? Should we keep these options easy to toggle?

I don't know if they were used, but I'm inclined to remove them if they were commented out.


OpenSim/Common/tests/CMakeLists.txt line 22 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Glad to see this reversed 👍

Done.


OpenSim/Simulation/SimbodyEngine/tests/CMakeLists.txt line 7 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

May not be related but I'd think tests under Simulation library would not need to link osimAnalyses

This was the case before these changes. I think this one could also be fixed in a follow up PR.

@nickbianco
nickbianco requested a review from aymanhab October 6, 2026 21:53
@nickbianco

Copy link
Copy Markdown
Member Author

@aymanhab merging per your previous approval and passing tests after addressing comments.

@nickbianco
nickbianco merged commit 7d853ff into main Oct 7, 2026
5 of 6 checks passed
@nickbianco
nickbianco deleted the add_test_macro branch October 7, 2026 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test data files are attached to only one test target

2 participants